[accname] Require specification of subtest count #42769
Conversation
The accname tests use a declarative style to express tests. This style is susceptible to false positives for author errors (e.g. invalid markup or typos in CSS selectors or class names). Extend the `verifyLabelsBySelector` utility function with a parameter for the expected number of tests in order to reduce the risk of false positives.
| verifyLabelsBySelector: function(expectedCount, selector) { | ||
| const els = document.querySelectorAll(selector); | ||
| if (!els.length) { | ||
| throw `Selector passed in verifyLabelsBySelector("${selector}") should match at least one element.`; | ||
| if (els.length !== expectedCount) { | ||
| throw `verifyLabelsBySelector expected ${expectedCount} elements but received ${els.length}`; |
There was a problem hiding this comment.
Thanks for filing this! I agree it's a good move to be more explicit about the expectations.
However, this is going to break a few outstanding PRs that expect selector to be first... I also would prefer selector remain the first argument (as the method name implies) and throw a warning if a count is not provided. And the error only if it is provided but doesn't match.
comp_label.html, comp_labelledby.html, and comp_text_node.html could then be left out of this PR to avoid conflicts/breakage with the outstanding PRs. They would throw a warning but not an error, and if this lands first, we could then update the other files in those PRs (or later ones) to use the optional-but-recommended expectedCount.
| @@ -32,7 +32,7 @@ <h2 class="ex" data-expectedlabel="heading label" data-testname="heading with te | |||
| <!-- Todo: test all remaining cases of https://w3c.github.io/accname/#comp_text_node --> | |||
|
|
|||
| <script> | |||
| AriaUtils.verifyLabelsBySelector(".ex"); | |||
| AriaUtils.verifyLabelsBySelector(1, ".ex"); | |||
There was a problem hiding this comment.
See below... Keep comp_text_node.html as-is (but with a warning) to make way for #42407
There was a problem hiding this comment.
#42407 was since merged so it's no longer necessary to leave this change out of the PR, but it should get the mentioned argument order change, and will need an update to the expectedCount integer.
| @@ -31,7 +31,7 @@ <h2 id="h">div group label</h2> | |||
| --> | |||
|
|
|||
| <script> | |||
| AriaUtils.verifyLabelsBySelector(".ex"); | |||
| AriaUtils.verifyLabelsBySelector(1, ".ex"); | |||
There was a problem hiding this comment.
See below... Keep comp_labelledby.html as-is (but with a warning) to make way for #42522
There was a problem hiding this comment.
#42522 was since merged so it's no longer necessary to leave this change out of the PR, but it should get the mentioned argument order change, and will need an update to the expectedCount integer.
| @@ -18,7 +18,7 @@ | |||
| <!-- Todo: test all remaining cases of https://w3c.github.io/accname/#comp_label --> | |||
|
|
|||
| <script> | |||
| AriaUtils.verifyLabelsBySelector(".ex"); | |||
| AriaUtils.verifyLabelsBySelector(1, ".ex"); | |||
There was a problem hiding this comment.
See below... Keep comp_label.html as-is (but with a warning) to make way for #41463
There was a problem hiding this comment.
@jugglinmike #41463 was also merged since the the time this was filed.
|
@jugglinmike Are you still working on this? Thanks. |
|
@cookiecrook Sure! Can you say a bit more about why you'd prefer the "expected" count to be optional? (My motivation is to prevent this avenue for author error, and I can't think of any benefits to allowing folks to risk the kind of false positives that are possible today.) |
|
I would expect most instances to use it, but making it optional (with a SHOULD warning) will allow an easier transition into using the new parameter. Making it optional allows you to check in the change without worrying about conflicts with the other open PRs (exception being #43740 due to its new parameter in the same function)... We can add follow-on PRs to any existing tests that are currently in flux. |
|
As an example, pretty much all of my comments above are about incoming PRs (some merged now) that would conflict with the PR as written. If you changed these two things, I believe it would smooth the transition:
|
|
FYI that the As part of this issue's work, we'll need to take into account the new implementations in I'm also working on creating common keyboard utilities with their own |
|
Assigning to Rahim to help finish this stale PR. |
|
FYI @janewman |
|
Thinking about this some more... This change would increase the likelihood of PR conflicts, and make other maintenance like reverting a commit less convenient... I'm closing as not-to-be-fixed. Please re-open if you disagree. |
Closes web-platform-tests/interop-accessibility#105
The accname tests use a declarative style to express tests. This style is susceptible to false positives for author errors (e.g. invalid markup or typos in CSS selectors or class names).
Extend the
verifyLabelsBySelectorutility function with a parameter for the expected number of tests in order to reduce the risk of false positives.Inspired by this test author error.